Allow use in same-origin children, add Feature Policy integration #13

anssiko · 2017-08-31T09:16:50Z

Per @clelland's suggestion in #10 (comment) this change allows Battery Status API use in same-origin children, and adds Feature Policy integration.

PTAL @clelland @RByers @mounirlamouri @riju

Fixes #10.

Preview | Diff

anssiko · 2017-09-19T08:15:15Z

@clelland @RByers @mounirlamouri, any concerns?

If I don't hear any, I'll merge this and @riju will update the Chrome's implementation accordingly.

RByers · 2017-09-19T15:25:32Z

Looks reasonable to me. Is the plan to add a feature control hook later?

clelland · 2017-09-19T15:59:39Z

The only forwards-incompatibility I can see with Feature Policy here is that if a deeply nested frame happens to be same-origin with the top-document, then it will be granted access to the API by default, while feature policy would disallow that.

That is, in an A->B->A embedding scenario, both the top-level and the innermost frame would be able to use the battery API. With feature policy, the inner frame wouldn't be allowed to use it, unless the top-level doc granted it to the middle frame, which then further granted it to the inner one.

(That is, feature policy would allow access to same-origin children -- and their same-origin children, and so on -- but not to arbitrary same-origin descendent frames)

mounirlamouri · 2017-09-20T00:31:05Z

Do we have any idea of how common it is to have x-origin iframe usage of the Battery API? Said otherwise, are we sure we are not breaking a real use case? Are we sure this change is even web compatible?

anssiko · 2017-09-20T12:13:00Z

Thanks for your comments!

Is the plan to add a feature control hook later?

@RByers, that was my initial plan. That said, we could consider adding the hook on the same pass, there's just one procedural issue that need to be cleared:

The Feature Policy spec is currently in incubation, and having an unstable normative reference might be an issue if we want to advance the Battery Status API to Candidate Recommendation and beyond.

@dontcallmedom to confirm whether we could make an exception here.

Do we have any idea of how common it is to have x-origin iframe usage of the Battery API?

@mounirlamouri, I'd like to know as well, but I don't have any data.

If we care strongly about this, would Chrome be fine with us adding telemetry to figure out how common x-origin iframe usage is?

A->B->A embedding scenario

@clelland, I'd expect many other platform features be similarly impacted? If so, I wouldn't be concerned about this case at hand specifically.

Couple of questions to @clelland et al.:

Feature Policy "v1" shipped in Chrome M62. Does it have all the features implemented we'd need for this spec to integrate with it?

Do you have established conventions for spec authors on how to hook into the Feature Policy spec the right way. I'm looking for normative spec language I could borrow similarly to e.g. https://w3c.github.io/webappsec-secure-contexts/#new This helps keep specs consistent.

Thanks!

dontcallmedom · 2017-09-20T12:20:48Z

re CR transition with a reference to an unstable spec - this won't block transition to CR (although we would want to highlight it in the transition request), but may block transition to PR. That being said, transition to PR needs a 2nd implementation we don't have yet.

anssiko · 2017-09-20T12:45:59Z

Thanks for the confirmation @dontcallmedom.

With that, I'd be happy to hook this spec into the Feature Policy spec when shown the preferred conventions in doing so.

clelland · 2017-09-21T00:30:27Z

@anssiko -- I think that we do have all of the features implemented in Chromium to do that. As long as the plan is just to reject the promise when disallowed, then it should be pretty simple.

I've written up a spec integration guide at https://github.com/WICG/feature-policy/blob/gh-pages/integration.md -- it's not strictly normative, but take a look and see if gives you enough guidance for the wording. If there's anything that I can add to that to help you out, I'm glad to do it.

anssiko · 2017-09-21T12:54:46Z

@clelland, thanks, the integration guide is very helpful.

I added Feature Policy integration in c2083ad, PTAL. See also the preview.

(I assumed the integration guide means "SecurityError" DOMException.)

clelland · 2017-09-22T04:46:29Z

@anssiko: Yes, that should totally read SecurityError DOMException, thanks :) PR fired. The actual error, though, should be whatever makes sense for the API -- whether rejecting a promise, or returning an error code, the important thing is just defining some way for the API to fail when disabled.

This change LGTM, though -- thanks!

anssiko · 2017-09-22T10:11:00Z

@mounirlamouri PTAL and merge this PR if you're happy.

anssiko · 2017-10-02T12:13:30Z

@mounirlamouri, gentle ping.

Others on this PR seem to be fine with this change, would like to get your explicit OK prior to merging.

anssiko · 2017-10-18T10:30:48Z

@mounirlamouri, we'll merge this PR unless we hear concerns from you by next Tuesday 24 Oct, to be followed by a corresponding patch to Chromium.

anssiko · 2017-10-23T07:12:09Z

@Honry, as we prepare for this PR to land, could you evaluate the gaps in the battery-status test suite (in the spirit of test as you commit, soon to be adopted for all deliverables), and update the test suite accordingly? Even if I see my name in OWNERS, I may not have the cycles to pull off the test suite update at this time, and I'd +1 you to become an owner for the battery status test suite.

On a quick glimpse, I think we need to update the iframe tests and add new tests the Feature Policy integration, and test interesting edge cases such as #13 (comment)

@foolip, this is another API that needs a bunch of manual tests due to its inherent nature, similarly to Vibration API. Is the following guide still considered the state of the art in manual w-p-t testing? :-)

http://web-platform-tests.org/writing-tests/manual.html

Honry · 2017-10-23T08:05:12Z

@anssiko, according to the test as you commit policy, would you please add a label "needs tests" to this PR?

I will add a corresponding PR in w-p-t to reflect the changes and involve my name in OWNERS file.
Then revisit current test coverage. :)

anssiko · 2017-10-23T08:45:13Z

would you please add a label "needs tests" to this PR?

Done. Thanks for your help updating this test suite too!

- Add feature-policy tests - Disallow using battery in cross-origin iframe - Allow using battery in same-origin iframe w3c/battery#13

Honry · 2017-10-23T12:18:48Z

I will add a corresponding PR in w-p-t to reflect the changes and involve my name in OWNERS file.

Done at web-platform-tests/wpt#7944, PTAL.

mounirlamouri · 2017-10-23T13:53:46Z

Sorry for the delay Anssi :( I still disagree with this change as I think it's curing thy symptoms, not the disease. The API should have an implementation that is privacy friendly.

This said, in practice, my main concern is whether this will break web pages. Was a use counter added into Chrome to discover how large the impact of this change would be and whether the deprecation is web-compatible? I don't think we can disable this in Chrome without this information.

clelland · 2017-10-23T14:05:16Z

@mounirlamouri, I don't think we've added a use counter for that into Chrome.

When we were looking at a similar behaviour change for the Fullscreen API, I think we added one that fired every time a same-origin page was blocked from entering fullscreen.

We could add a similar counter here for battery-status, as well as one that fires when a same-origin-as-root-but-embedded-cross-origin page successfully uses the API. If we do it quickly enough, we should be be able to get that change merged for M63, to get some actual data before the end of the year.

mounirlamouri · 2017-10-23T14:08:31Z

I think something that checks the usage in x-origin iframe and another for same origin should be enough.

clelland · 2017-10-23T15:46:08Z

So Chrome doesn't currently restrict getBattery at all. Fully complying with this change would block the API in Chrome:

in all insecure contexts
in any frame which is at a different origin than its parent (without an appropriate feature policy)
in any frame which is at a different origin than the top-level document (again, without an appropriate feature policy)

If we add counters for those situations, then we should have a good idea how much usage would be broken by adopting this change (and also adopting the current spec as-is).

(Also, this is Chrome-specific, but both caniuse and the API confluence dashboard suggest that the API is not implemented anywhere else -- if that's wrong, it could change the 'web-compatible' calculation quite a bit)

anssiko · 2018-09-18T11:22:36Z

(Here's an HTML diff comparing this branch with gh-pages to ease review, e.g. at F2F. I enabled pr-preview to automate this for any subsequent PRs.)

clelland · 2018-09-18T14:06:58Z

index.html

-        promise</a> with a "<a>SecurityError</a>" <a>DOMException</a>, return
-        this <a>Navigator</a> object's <a>battery promise</a> and abort these
-        steps.
+        <li>If this <a>Navigator</a> object's <a>relevant global object</a>'s


With feature policy integration, I think you can replace this entire paragraph with something like

If this Navigator object's relevant global object's associated Document is not allowed to use the battery feature, then reject...

Because the default allowlist is 'self', that will automatically take care of the same-origin embed case, while allowing cross-origin usage only if explicitly enabled by the embedding document (which this paragraph still prohibits, I think)

I pushed an update in an attempt to make use of "allowed to use". Please take a look. (I'm happy to see "allowed to use" abstracted out and reusable.)

clelland · 2018-09-18T14:10:04Z

index.html

+        The Battery Status API is a <a>policy-controlled feature</a>, as
+        defined by Feature Policy [[!FEATURE-POLICY]]. The <a>feature name</a>
+        for the Battery Status API is "<code>battery</code>". The <a>default
+        allowlist</a> for the Battery Status API is <code>« "self" »</code>.


Minor nit: the value for the default allowlist would be 'self', with single quotes included

You can just say now

The Battery Status API is a policy-controlled-feature identified by the string "battery". It's default allowlist is 'self'.

Ditto, pushed an update per your proposal.

- Use 'allowed to use' as a condition for rejection, move the old text to a note - Align Feature Policy integration section with the latest prose - Fix allowlist notation

clelland

One small comment about the non-normative explanation; this looks great otherwise.

clelland · 2018-10-02T14:54:02Z

index.html

+            <code>Document</code></a>'s <a>browsing context</a>'s <a>active
+            document</a>'s <a>origin</a> is not <a>same origin-domain</a> with
+            the <a>origin</a> of the <a>current settings object</a> of this
+            <a>Navigator</a> object.


You could add something like "unless specifically allowed by the document's feature policy." to this, if you wanted to be clear that it's possible to grant access to cross-origin subframes, but you have to be deliberate about it. The default situation is exactly as you describe here.

Note it is possible to explicitly grant access to cross-origin subframes using feature policy, in which case the promise is not rejected.

anssiko · 2018-10-03T10:28:06Z

@clelland, thanks for your comments! I integrated your suggestion and pushed an update. At F2F, we can gauge whether we have consensus to land this and update the implementation(s) accordingly. I invited @mounirlamouri to join as well.

anssiko · 2018-10-21T05:40:42Z

index.html

+      </h2>
+      <p data-link-for="Navigator">
+        The Battery Status API is a <a>policy-controlled feature</a> identified
+        by the string "<code>battery</code>". It's default allowlist is


(Note to self: s/It’s/Its/)

anssiko · 2018-10-22T08:04:59Z

F2F Resolution: https://www.w3.org/2018/10/22-dap-minutes.html#x05

anssiko · 2019-06-26T18:40:49Z

(Rebased with gh-pages and fixed merge conflicts in a9e3c94.)

anssiko · 2019-08-16T12:02:44Z

Time for the annual update to this issue ☺️

There are the following battery use counters invoked in getBattery.

In percentages of Chrome page loads (across all channels and platforms) that use the corresponding feature at least once, as of today:

css-meeting-bot · 2019-09-20T02:29:08Z

The Devices and Sensors Working Group just discussed Battery Status, and agreed to the following:

RESOLUTION: Replace SecurityError with NotAllowedError in step 2 of getBattery() algorithm and merge #13.

The full IRC log of that discussion

<reillyg> Topic: Battery Status
<anssik> github: https://github.com//pull/13
<reillyg> Anssi: In Chrome the battery status API is currently available in cross-origin iframes.
<reillyg> ... Mozilla had concerns about this API and dropped it a long time ago.
<reillyg> Tom: Looking at stats on HTTP Archive ads are the biggest user of this cross-origin.
<reillyg> Reilly: It doesn't seem like we've seen a clearly articulated reason not to land this PR.
<tomayac> Usage stats: https://chromestatus.com/metrics/feature/timeline/popularity/2198
<reillyg> Anssi: The issue is breaking sites.
<reillyg> flaki: If you have metrics for APIs like this then from a standardization point of view we should provide more restrictive versions of these APIs.
<reillyg> ... For example, "is on battery power" vs. "what is the current battery level"
<reillyg> Anssi: In retrospect, agreed.
<reillyg> ... The current usage is very high for taking this API away for cross-origin contexts.
<reillyg> Reilly: Ian, do we have exerience taking away APIs like this with FP in other contexts?
<reillyg> ... What has been the reaction?
<reillyg> Ian: Geolocation is currently under feature policy.
<reillyg> Tom: We didn't hear massive outcry.
<reillyg> Ian: It's easy to fix in legitimate use cases.
<reillyg> Anssi: Do we know what will break?
<reillyg> Reilly: chromestatus.com includes top URLs using the API.
<tomayac> The post from geolocation days: https://developers.google.com/web/updates/2016/04/geolocation-on-secure-contexts-only
<reillyg> Reilly: We should post an Intent to Implement/Deprecate and just do it.
<reillyg> Ian: Some people think we should just cut it down to a small number of buckets.
<reillyg> Reilly: I think we should do both.
<tomayac> Intent to deprecate: insecure usage of powerful features: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2LXKVWYkOus/gT-ZamfwAKsJ
<reillyg> Ian: Is the proposal still to never resolve the promise?
<reillyg> Reilly: Why not reject with NotAllowedError?
<reillyg> Anssi: For compatibility with existing content.
<reillyg> Reilly: Shouldn't script have already been written to handle other failures?
<reillyg> Ian: We could browse the web with canary running with this disabled and see how bad it is.
<reillyg> Reilly: Both mechanisms will break content but never resolving will break content in weirder ways.
<tomayac> and wronger ways
<reillyg> Reilly: The current PR text rejects with a SecurityError. It should be a NotAllowedError but otherwise is the right behavior.
<reillyg> PROPOSED RESOLUTION: Replace SecurityError with NotAllowedError and merge existing PR.
<reillyg> PROPOSED RESOLUTION: Replace SecurityError with NotAllowedError and merge #13.
<reillyg> PROPOSED RESOLUTION: Replace SecurityError with NotAllowedError in step 2 of getBattery() algorithm and merge #13.
<reillyg> Anssi: Any concerns given wide-scale implications?
<reillyg> [None heard]
<reillyg> Ian: Are there other interfaces in the Battery API which should be gated by FP?
<reillyg> Anssi: getBattery() is the only entrypoint.
<reillyg> RESOLUTION: Replace SecurityError with NotAllowedError in step 2 of getBattery() algorithm and merge #13.

Error message: Couldn't match "EventTarget" to anything in the document or in any other document cited in this specification: webidl

…ttery into issue-10-secure-top-level

anssiko · 2019-09-24T12:31:15Z

Merging this PR per F2F resolution #13 (comment). This branch includes 3e08915 that was part of the resolution.

- Add feature-policy tests - Disallow using battery in cross-origin iframe - Allow using battery in same-origin iframe w3c/battery#13

* Add feature-policy to battery-status - Add feature-policy tests - Disallow using battery in cross-origin iframe - Allow using battery in same-origin iframe w3c/battery#13 * Replace SecurityError with NotAllowedError and use modern ES6

anssiko · 2019-09-25T12:33:31Z

Thanks to @Honry, now also Feature Policy section is updated. Consider #24 to be a part of this PR.

…estonly Automatic update from web-platform-tests Add feature-policy to battery-status (#7944) * Add feature-policy to battery-status - Add feature-policy tests - Disallow using battery in cross-origin iframe - Allow using battery in same-origin iframe w3c/battery#13 * Replace SecurityError with NotAllowedError and use modern ES6 -- wpt-commits: fab60f68f7227113a171fb8e17d84d2f5983109b wpt-pr: 7944

…estonly Automatic update from web-platform-tests Add feature-policy to battery-status (#7944) * Add feature-policy to battery-status - Add feature-policy tests - Disallow using battery in cross-origin iframe - Allow using battery in same-origin iframe w3c/battery#13 * Replace SecurityError with NotAllowedError and use modern ES6 -- wpt-commits: fab60f68f7227113a171fb8e17d84d2f5983109b wpt-pr: 7944 UltraBlame original commit: f722b65ec0ba6824338b4d3e9d5a62e2a28011ad

anssiko · 2019-10-16T15:16:15Z

web-platform-tests Add feature-policy to battery-status web-platform-tests/wpt#7944
Chromium https://crbug.com/1007264

anssiko mentioned this pull request Aug 31, 2017

Allow use from within secure context and top-level browsing context only #10

Closed

anssiko force-pushed the issue-10-secure-top-level branch from a2761ae to c2083ad Compare September 21, 2017 12:52

anssiko changed the title ~~Allow use in same-origin children~~ Allow use in same-origin children, add Feature Policy integration Oct 9, 2017

anssiko mentioned this pull request Oct 9, 2017

Relation to Feature Policy w3c/sensors#133

Closed

anssiko requested a review from mounirlamouri October 18, 2017 10:30

anssiko added the needs tests label Oct 23, 2017

Honry added a commit to Honry/web-platform-tests that referenced this pull request Oct 23, 2017

Add feature-policy to battery-status

f66e903

- Add feature-policy tests - Disallow using battery in cross-origin iframe - Allow using battery in same-origin iframe w3c/battery#13

Honry mentioned this pull request Oct 23, 2017

Add feature-policy to battery-status web-platform-tests/wpt#7944

Merged

anssiko mentioned this pull request Oct 23, 2017

Adopt 'test as you commit' policy #14

Merged

anssiko mentioned this pull request Sep 18, 2018

DAS WG TPAC F2F agenda proposals w3c/devicesensors-wg#22

Closed

clelland reviewed Sep 18, 2018

View reviewed changes

Use 'allowed to use' abstraction, reword Feature Policy integration

fb43978

- Use 'allowed to use' as a condition for rejection, move the old text to a note - Align Feature Policy integration section with the latest prose - Fix allowlist notation

clelland reviewed Oct 2, 2018

View reviewed changes

Clarify note on rejection condition

e486c5a

Note it is possible to explicitly grant access to cross-origin subframes using feature policy, in which case the promise is not rejected.

anssiko commented Oct 21, 2018

View reviewed changes

anssiko mentioned this pull request Jun 17, 2019

DAS WG TPAC F2F agenda (Fukuoka, Sep 19-20 2019) w3c/devicesensors-wg#24

Closed

Merge branch 'gh-pages' into issue-10-secure-top-level

a9e3c94

anssiko added 4 commits September 24, 2019 15:19

Replace SecurityError with NotAllowedError

3e08915

Define EventTarget reference to fix a ReSpec error

7ad0dd5

Error message: Couldn't match "EventTarget" to anything in the document or in any other document cited in this specification: webidl

Merge branch 'issue-10-secure-top-level' of https://github.com/w3c/ba…

e500e98

…ttery into issue-10-secure-top-level

Remove duplicate EventTarget definition

6d2ce77

anssiko merged commit 5a628d9 into gh-pages Sep 24, 2019

Honry added a commit to Honry/web-platform-tests that referenced this pull request Sep 25, 2019

Add feature-policy to battery-status

dfa3f0f

- Add feature-policy tests - Disallow using battery in cross-origin iframe - Allow using battery in same-origin iframe w3c/battery#13

anssiko mentioned this pull request May 28, 2024

Are there new use cases for battery API? #64

Open

Allow use in same-origin children, add Feature Policy integration #13

Allow use in same-origin children, add Feature Policy integration #13

Conversation

anssiko commented Aug 31, 2017 • edited by pr-preview bot Loading

anssiko commented Sep 19, 2017

RByers commented Sep 19, 2017

clelland commented Sep 19, 2017 • edited Loading

mounirlamouri commented Sep 20, 2017

anssiko commented Sep 20, 2017

dontcallmedom commented Sep 20, 2017

anssiko commented Sep 20, 2017

clelland commented Sep 21, 2017

anssiko commented Sep 21, 2017

clelland commented Sep 22, 2017

anssiko commented Sep 22, 2017

anssiko commented Oct 2, 2017

anssiko commented Oct 18, 2017

anssiko commented Oct 23, 2017

Honry commented Oct 23, 2017 • edited Loading

anssiko commented Oct 23, 2017

Honry commented Oct 23, 2017

mounirlamouri commented Oct 23, 2017

clelland commented Oct 23, 2017

mounirlamouri commented Oct 23, 2017

clelland commented Oct 23, 2017

anssiko commented Sep 18, 2018

clelland Sep 18, 2018

Choose a reason for hiding this comment

anssiko Oct 2, 2018

Choose a reason for hiding this comment

clelland Sep 18, 2018

Choose a reason for hiding this comment

anssiko Oct 2, 2018

Choose a reason for hiding this comment

clelland left a comment

Choose a reason for hiding this comment

clelland Oct 2, 2018

Choose a reason for hiding this comment

anssiko commented Oct 3, 2018

anssiko Oct 21, 2018

Choose a reason for hiding this comment

anssiko commented Oct 22, 2018

anssiko commented Jun 26, 2019

anssiko commented Aug 16, 2019

css-meeting-bot commented Sep 20, 2019

anssiko commented Sep 24, 2019

anssiko commented Sep 25, 2019

anssiko commented Oct 16, 2019

anssiko commented Aug 31, 2017 •

edited by pr-preview bot

Loading

clelland commented Sep 19, 2017 •

edited

Loading

Honry commented Oct 23, 2017 •

edited

Loading